Refactor + resolve & persist docs errors#175
Conversation
….com/fivetran/dbt_jira into refactor/resolve-persist-docs-errors
fivetran-jamie
left a comment
fivetran-jamie
left a comment
There was a problem hiding this comment.
Looks great! Couple of in-line questions, plus this:
Should we remove the hard-coded story point logic from the timestamp_issue_field_history model and its intermediate as well? Things work fine with it still in there as it's dynamic, but maybe we want to totally clean it up
macros/split_sprint_ids.sql
Outdated
| cast(daily_issue_field_history.story_points as {{ dbt.type_numeric() }}) as story_points, | ||
| {% endif %} | ||
| {% if include_story_point_estimate %} | ||
| cast(daily_issue_field_history.story_point_estimate as {{ dbt.type_numeric() }}) as story_point_estimate, | ||
| {% endif %} |
There was a problem hiding this comment.
I see why it's ideal that these fields be numerics, but it makes me a little nervous to cast from string to numeric, at least without doing some cleaning first (that is, unless we can confirm that these values will always be formatted numerically)
Perhaps we'd wanna apply a macro to ensure only numeric characters are included before safely casting? We have a similar macro in Amazon Selling Partner.
There was a problem hiding this comment.
Good call. Updated.
macros/split_sprint_ids.sql
Outdated
| {% set issue_field_history_columns = var('issue_field_history_columns', []) | map('lower') | list %} | ||
| {% set include_story_points = 'story points' in issue_field_history_columns %} | ||
| {% set include_story_point_estimate = 'story point estimate' in issue_field_history_columns %} |
There was a problem hiding this comment.
Since this is repeated, I wonder if it's best to do this logic in the model itself and then pass include_story_points and include_story_point_estimate as macro parameters?
There was a problem hiding this comment.
Refactored.
models/staging/stg_jira__sprint.sql
Outdated
| select * | ||
| select * | ||
| from {{ ref('stg_jira__sprint_tmp') }} | ||
| where not coalesce(_fivetran_deleted, false) |
There was a problem hiding this comment.
could you move this to the end or the final CTE? Just in case anyone doesn't have _fivetran_deleted, the null version would get created in the fields CTE
There was a problem hiding this comment.
Moved along with stg_jira__issue
fivetran-avinash
left a comment
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie good call with refactoring the timestamp models. Ready for re-review.
fivetran-jamie
left a comment
fivetran-jamie
left a comment
There was a problem hiding this comment.
Approved with some changelog comments!
CHANGELOG.md
Outdated
| - Refactors the `split_sprint_ids` macro to accept `include_story_points` and `include_story_point_estimate` as explicit boolean parameters rather than re-evaluating `var('issue_field_history_columns')` inside each dispatch variant. | ||
| - Removes hard-coded story point conditional blocks from `int_jira__timestamp_field_history_scd`. Story point columns now flow through the generic `custom_columns` loop alongside other user-defined field history columns. | ||
| - Pre-computes `include_story_points` and `include_story_point_estimate` at the top of `jira__timestamp_issue_field_history` to avoid repeated `var()` evaluations throughout the model. |
There was a problem hiding this comment.
Not sure if we need to document all this, particularly the last two
CHANGELOG.md
Outdated
| ## Under the Hood | ||
| - Moved the `_fivetran_deleted` filter in `stg_jira__issue` to the `final` CTE so it runs after `fill_staging_columns` null-fills the column for sources that don't have `_fivetran_deleted` records. |
There was a problem hiding this comment.
Not sure if we need to document this here -- also looks like there are 2 under the hood sections
fivetran-avinash
added
docs:ready
3 participants
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog